Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safari client spike #75

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft

Safari client spike #75

wants to merge 34 commits into from

Conversation

leesio
Copy link
Contributor

@leesio leesio commented Jan 20, 2021

PoC for Safari Web Notifications

@leesio leesio force-pushed the safari-client-spike branch from 4de4d64 to d9205d3 Compare January 22, 2021 16:34
Copy link
Contributor

@TomKemp TomKemp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good. I've left a few comments for discussion.

return RegistrationState.PERMISSION_PROMPT_REQUIRED;
}

async clearAllState() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the purpose of this method is for non-safari web notifications 🤔 I doubt you would want to show the permission request dialog box when clearing the state. I guess the behaviour of this method only makes sense if it assumes that the user has already given permission? And therefore it probably doesn't matter that this won't successfully ask for permission in Safari, because developers probably wouldn't want to do that. If that is the assumption then we should probably make that explicit with checks/errors. I can explain that if it didn't make sense.

src/safari-client.js Outdated Show resolved Hide resolved
}

async start() {
await this._resolveSDKState();
Copy link
Contributor

@TomKemp TomKemp Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should remove this call here, which detects subscription changes, so that the behaviour is consistent with Safari. Otherwise, developers might get bugs where the state SDK is different at different points in their code when running in Safari vs other browsers.

@leesio leesio force-pushed the safari-client-spike branch from d3a14d4 to 280ae74 Compare February 17, 2021 18:35
@leesio leesio force-pushed the safari-client-spike branch from 4cf14ef to d672b11 Compare February 18, 2021 13:06
James Lees added 15 commits February 18, 2021 18:12
Safari devices will include the web push ID
Dummy method for requesting website push id
Move shared config into base client
Add not implemented methods to abstract base class
The state management is a little bit different to on web-push because
the device token should (almost) never change and we can't freely check
the stored token before requesting permission (because of how strict the
safari is at enforcing permission request from a user gesture).

This means the start behaves a little differently:
- if the permission is default, immediately request permission
- register the device
- update the internal state
James Lees added 5 commits February 26, 2021 13:48
@leesio leesio force-pushed the safari-client-spike branch from 67ee6aa to 993766c Compare March 4, 2021 17:07
James Lees and others added 3 commits March 22, 2021 19:01
- always return self from start method
- start method should fail if permission request throws
- use this._platform when setting user id
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants